Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spec): Requirements on valid(v) #748

Merged
merged 8 commits into from
Jan 14, 2025
Merged

Conversation

josef-widder
Copy link
Member

Closes: #510


PR author checklist

For all contributors

For external contributors

@josef-widder josef-widder requested a review from cason as a code owner January 10, 2025 13:13
@josef-widder josef-widder added the spec Related to specifications label Jan 10, 2025
Copy link
Contributor

@cason cason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

Most of the suggestions I made were to align the content of this section with the "format" used in the remainder of this document.

Regarding the last sub(*)section, from line 540, I think it should not be part of this document. The same for the linked commend in issue 510.

They end up being too much specific for the level of detail that is considered in this document.

specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
specs/consensus/overview.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel <[email protected]>
Signed-off-by: Josef Widder <[email protected]>
@josef-widder
Copy link
Member Author

Regarding the last sub(*)section, from line 540, I think it should not be part of this document. The same for the linked commend in issue 510.

They end up being too much specific for the level of detail that is considered in this document.

I agree. We might do another md file with implementation-specific discussions, and link it.

More generally, I find the document very long. I wonder whether we should cut it in multiple shorter ones that might be easier to navigate if we have a good top-level document...

@cason
Copy link
Contributor

cason commented Jan 13, 2025

More generally, I find the document very long.

For an "overview" document it is definitely, ehe.

But I wonder whether we should try to split it now or first have on it all content we find relevant, then do the splitting?

@josef-widder
Copy link
Member Author

For an "overview" document it is definitely, ehe.

But I wonder whether we should try to split it now or first have on it all content we find relevant, then do the splitting?

Let's fix the TODO on soft upgrades on our call tomorrow. Then merge, and open an issue about splitting.

@josef-widder josef-widder added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit d07ff16 Jan 14, 2025
2 checks passed
@josef-widder josef-widder deleted the josef/i510-validity branch January 14, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Related to specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spec: Formally define the valid(v) properties
2 participants